linux, checking THP current system settings.#494
linux, checking THP current system settings.#494devnexen wants to merge 1 commit intomicrosoft:devfrom
Conversation
daanx
left a comment
daanx
left a comment
There was a problem hiding this comment.
Thanks for adding this (the current pipeline failure is due to an earlier commit of mine I think for Windows). I added some inline comments. Also, as I understand, the madvise call is only needed if the setting is madvise ?
src/os.c
Outdated
|
|
||
| static void os_detect_transparent_huge_pages(void) { | ||
| #if defined(MADV_HUGEPAGE) | ||
| int fd = open("/sys/kernel/mm/transparent_hugepage/enabled", O_RDONLY); |
There was a problem hiding this comment.
Is this path always fixed? I read here that it can also be /sys/kernel/mm/redhat_transparent_hugepage/enabled for example?
src/os.c
Outdated
| close(fd); | ||
| if (nread >= 1) { | ||
| // in some configurations, it can occur (e.g. embedded) | ||
| if (!strstr(buf, "[never]")) os_transparent_huge_pages = true; |
There was a problem hiding this comment.
What are the valid contents here? I guess always, madvise and never.
But it seems also [never] is valid? Should we just check for madvise perhaps?
devnexen
force-pushed
the
madv_hugepage
branch
from
eccbff7 to
1ab104e
Compare
src/os.c
Outdated
|
|
||
| for (i = 0; i < sizeof(paths) / sizeof(paths[0]); i ++) { | ||
| fd = open(paths[i], O_RDONLY); | ||
| if (fd > 0) break; |
There was a problem hiding this comment.
open can return 0, that's a valid value, not an error.
So I would change that check to fd >= 0
There was a problem hiding this comment.
in general yes even tough stdin might not be closed yet at this early stage but nevertheless I made it less ambiguous.
devnexen
force-pushed
the
madv_hugepage
branch
from
1ab104e to
83228bb
Compare
|
Ah, this is starting to look rather complicated and it is unclear whether we actually need it. Currently, it is only called if we cannot do it directly and via |
|
I see your point, that is fine if you prefer not merging it. |
| ssize_t nread = read(fd, &buf, sizeof(buf)); | ||
| close(fd); | ||
| if (nread >= 1) { | ||
| if (strstr(buf, "[madvise]")) os_transparent_huge_pages = true; |
There was a problem hiding this comment.
buf is not null-terminated, so strstr is undefined behavior. memmem(buf, nread, "[madvise]", strlen("[madvise]")) instead?
4 participants
No description provided.